Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extract block mover buttons so that they can be individually imported #22122

Merged
merged 2 commits into from
May 7, 2020
Merged

Extract block mover buttons so that they can be individually imported #22122

merged 2 commits into from
May 7, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented May 6, 2020 •
edited
Loading

Description

The Block mover buttons are exported as a single component. This PR allows the up and down buttons to be individually imported.

This change is required as part of #18014, which manages focus on the buttons using a roving tab index within the block navigator.

How has this been tested?

This is just a refactor, so no changes. Block movers should continue to look and work as they currently do.

Types of changes

Refactor

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@talldan talldan added [Type] Enhancement A suggestion for improvement. General Interface Parts of the UI which don't fall neatly under other labels. labels May 6, 2020
@talldan talldan self-assigned this May 6, 2020
@github-actions
Copy link

github-actions bot commented May 6, 2020 •
edited
Loading

Size Change: +198 B (0%)

Total Size: 821 kB

Filename Size Change
build/block-editor/index.js 101 kB +187 B (0%)
build/block-editor/style-rtl.css 10.2 kB +6 B (0%)
build/block-editor/style.css 10.2 kB +5 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.6 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-library/editor-rtl.css 7.08 kB 0 B
build/block-library/editor.css 7.08 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.24 kB 0 B
build/block-library/style.css 7.25 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 179 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 4.05 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.3 kB 0 B
build/edit-site/style-rtl.css 5.19 kB 0 B
build/edit-site/style.css 5.2 kB 0 B
build/edit-widgets/index.js 8.37 kB 0 B
build/edit-widgets/style-rtl.css 4.68 kB 0 B
build/edit-widgets/style.css 4.68 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@talldan talldan mentioned this pull request May 6, 2020
Merged
5 tasks
@mtias
Copy link
Member

mtias commented May 6, 2020

Do you think this still makes sense considering possible work in #21935 ?

adamziel
adamziel approved these changes May 6, 2020
View reviewed changes
Copy link
Contributor

@adamziel adamziel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good and works well so I'm approving. I'll leave the decision whether or not to merge this (as per the previous comment) up to @talldan .

@talldan
Copy link
Contributor Author

talldan commented May 7, 2020 •
edited
Loading

@mtias I don't see much difference in #21935, the buttons in the movers would still need to have their focus managed individually.

Having said that, there's some parallel work being done on the toolbar that has similar goals, but addresses this differently - #20008.

(cc @diegohaz - it looks like our changes will conflict, this PR is to support mover buttons in the Block Navigator where they wouldn't be ToolbarButtons. I think we should be able to make our code work together. We could consider something like <BlockMoverButton as={ ToolbarButton } /> and also expose that for the BlockMover component, or make ToolbarButton the default and make it work the other way around, that would make sense).

@talldan
Copy link
Contributor Author

talldan commented May 7, 2020

I've tested the PR again, and I can't see any issues.

I'm going to move forward and merge this, it's a low risk change.

@talldan talldan merged commit d433129 into master May 7, 2020
@talldan talldan deleted the update/individual-block-mover-button branch May 7, 2020 06:21
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamziel adamziel adamziel approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@talldan talldan

Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 8.1
Development

Successfully merging this pull request may close these issues.
3 participants
@talldan @mtias @adamziel